Skip to content

Conversation

@W3lac3
Copy link
Contributor

@W3lac3 W3lac3 commented Nov 7, 2024

This PR will allow you to pass 4x4 lua tables in the parameter of the setElementBoneMatrix function, currently it only accepts one Matrix, which makes it difficult to use this function.

}

bool CLuaPedDefs::SetElementBoneMatrix(lua_State* const luaVM, CClientPed* entity, std::uint32_t boneId, CMatrix boneMatrix)
bool CLuaPedDefs::SetElementBoneMatrix(lua_State* const luaVM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wont work for argument parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it would be through LUA_DECLARE, correct? I've made the change, if that's all it is, I'll send.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you would have to remove argument parser from the function list. Try to find a different way instead of not using argparser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to follow the declaration pattern of other functions. I've sent the changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check if it works as intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I never send a commit without testing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didnt check your first commit though 😒

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did. All the parameters were working and the function worked correctly. I only changed it because you told me to, but it was already working.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you say so, but please try figuring out a way with ArgumentParser and not lua bindings if possible

return std::make_tuple(x, y, z, w);
}

bool CLuaPedDefs::SetElementBoneMatrix(lua_State* const luaVM, CClientPed* entity, std::uint32_t boneId, CMatrix boneMatrix)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just implement that "CMatrix" can be read from 4x4 lua table and don't touch this file at all

@Lpsd
Copy link
Member

Lpsd commented Nov 7, 2024

Why do you need to use the old argument parser here? Can you not use std::variant with the new arg parser instead of the old argStream.NextIsTable() approach?

Anyway, I think @CrosRoad95 suggestion is a more sensible approach as it is wider reaching, for around the same amount of effort.

@W3lac3
Copy link
Contributor Author

W3lac3 commented Nov 8, 2024

Why do you need to use the old argument parser here? Can you not use std::variant with the new arg parser instead of the old argStream.NextIsTable() approach?

Anyway, I think @CrosRoad95 suggestion is a more sensible approach as it is wider reaching, for around the same amount of effort.

I think it's really the best solution to change the ArgumentParse to convert the 4x4 lua table into a CMatrix. But as it's something more global, not exactly linked to a function, it would be better to open a new PR, where it changes to allow passing the 4x4 lua table in CMatrix and close this PR.

I've made this change where it will also allow you to pass a 4x4 lua table, but I haven't sent it yet, because I want to make sure that the best option would be to open another PR, if so, I'll open it by making this change.

@W3lac3 W3lac3 closed this Nov 9, 2024
@W3lac3 W3lac3 deleted the fix/setElementBoneMatrix branch November 9, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants